chore: Modernize squid-mixin #1526
Conversation
|
|
postnati
changed the title
postnati
changed the title
schmikei
left a comment
schmikei
left a comment
There was a problem hiding this comment.
Looking closer! Think I've caught a couple things that we should fix though. Will sync up with you async on these
| template-job-rule: | ||
| reason: "Prometheus datasource variable is being named as prometheus_datasource now while linter expects 'datasource'" | ||
| panel-datasource-rule: | ||
| reason: "Modern mixins use signal-based architecture where datasource references are handled by the framework" |
There was a problem hiding this comment.
I think this is wrong. Ideally our refactor shouldn't really touch the .lint file
| this.grafana.rows.serverRow, | ||
| ] | ||
| + | ||
| if this.config.enableLokiLogs then |
There was a problem hiding this comment.
I think we should be using the logs-lib library here instead of combined logs/metrics panels.
| @@ -0,0 +1,3 @@ | |||
| // grafonnet must be imported with "g" alias | |||
| local g = import './vendor/github.com/grafana/grafonnet/gen/grafonnet-v11.0.0/main.libsonnet'; | |||
| g | |||
There was a problem hiding this comment.
| g | |
| // grafonnet must be imported with "g" alias | |
| local g = import './vendor/github.com/grafana/grafonnet/gen/grafonnet-v11.0.0/main.libsonnet'; | |
| g |
This is the wrong import syntax: example https://github.com/grafana/jsonnet-libs/blob/master/apache-activemq-mixin/g.libsonnet
| { | ||
| new(this): | ||
| { | ||
| overview: { |
There was a problem hiding this comment.
This is the incorrect way of structuring the dashboard link.
Follow the pattern here: https://github.com/grafana/jsonnet-libs/blob/master/apache-activemq-mixin/links.libsonnet
| "links": [ ], | ||
| "links": [ | ||
| { | ||
| "keepTime": true, |
There was a problem hiding this comment.
Squid overview has a link to itself? I think that is incorrect and probably indicative of something wrong in links.libsonnet.
| multiclusterSelector: 'job=~"$job"', | ||
| squidSelector: if self.enableMultiCluster then 'job=~"$job", cluster=~"$cluster"' else 'job=~"$job"', | ||
| // Basic filtering | ||
| filteringSelector: 'job=~"$job", instance=~"$instance"', |
There was a problem hiding this comment.
I think this is the incorrect filtering selector. I believe its default should probably be
| filteringSelector: 'job=~"$job", instance=~"$instance"', | |
| filteringSelector: 'job="integrations/squid"', |
| "uid": "${datasource}" | ||
| }, | ||
| "expr": "rate(squid_server_other_kbytes_in_kbytes_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])", | ||
| "expr": "avg by (job,instance) (\n rate(squid_server_other_kbytes_in_kbytes_total{job=~\"$job\", instance=~\"$instance\",job=~\"$job\",instance=~\"$instance\"}[$__rate_interval])\n)", |
There was a problem hiding this comment.
Before we weren't averaging this. Was this an intentional change?
| "uid": "${datasource}" | ||
| }, | ||
| "expr": "rate(squid_server_http_kbytes_in_kbytes_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])", | ||
| "expr": "avg by (job,instance) (\n rate(squid_server_http_kbytes_in_kbytes_total{job=~\"$job\", instance=~\"$instance\",job=~\"$job\",instance=~\"$instance\"}[$__rate_interval])\n)", |
There was a problem hiding this comment.
Same thing here about averaging when before we were doing a pure rate.
| "uid": "${datasource}" | ||
| }, | ||
| "expr": "rate(squid_swap_outs_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])", | ||
| "expr": "avg by (job,instance) (\n rate(squid_swap_outs_total{job=~\"$job\", instance=~\"$instance\",job=~\"$job\",instance=~\"$instance\"}[$__rate_interval])\n)", |
| "uid": "${datasource}" | ||
| }, | ||
| "expr": "rate(squid_swap_ins_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])", | ||
| "expr": "avg by (job,instance) (\n rate(squid_swap_ins_total{job=~\"$job\", instance=~\"$instance\",job=~\"$job\",instance=~\"$instance\"}[$__rate_interval])\n)", |
There was a problem hiding this comment.
will let you read through the diff here but similar about avg when should just keep the rate.
3 participants
This PR modernizes the squid-mixin to use grafonnet v11 and the signals architecture pattern.